cc_toolchain's tool_map should be cfg-exec-configured #555
Conversation
dzbarsky
requested review from
armandomontanez,
c-mita,
comius,
hvadehra,
pzembrod and
trybka
as code owners
dzbarsky
force-pushed
the
zbarsky/tool_map
branch
15 times, most recently
from
ca23f76 to
a91a95d
Compare
dzbarsky
force-pushed
the
zbarsky/tool_map
branch
7 times, most recently
from
63ff83e to
cc175a4
Compare
dzbarsky
force-pushed
the
zbarsky/tool_map
branch
4 times, most recently
from
0ac92cf to
ad72f0e
Compare
armandomontanez
left a comment
armandomontanez
left a comment
There was a problem hiding this comment.
Thanks for putting this together! I'll try to take a closer look early next week. If I don't get back to you in a timely manner, please ping me with a comment here.
dzbarsky
force-pushed
the
zbarsky/tool_map
branch
3 times, most recently
from
21a6ab9 to
79f0585
Compare
dzbarsky
force-pushed
the
zbarsky/tool_map
branch
from
79f0585 to
2574ba4
Compare
@armandomontanez PTAL :) |
armandomontanez
left a comment
armandomontanez
left a comment
There was a problem hiding this comment.
A few more small things, but overall looks great!
The macOS build is failing because macOS is currently doing a cross-compile to Linux and the tools are now exec-configured. This means llvm-libtool-darwin is getting selected as the archiver, but we end up using ar-style flags. Easy fix is to just merge the two tool maps to always use llvm-ar instead of llvm-libtool-darwin:
A number of projects prefer ar style archivers for cross-language interop reasons anyways.
| known_features = known_features, | ||
| enabled_features = enabled_features, | ||
| compiler = compiler, | ||
| cpu = select({ |
There was a problem hiding this comment.
Please move these to a shared constant rather than copying. These CPU values are arcane, so minimizing duplication is important.
| @@ -0,0 +1,46 @@ | |||
| # Copyright 2018 The Bazel Authors. All rights reserved. | |||
There was a problem hiding this comment.
Copyright year needs to be current year for new files.
| } | CC_TOOLCHAIN_CONFIG_PUBLIC_ATTRS | { | ||
| # Override tool_map to make it optional. | ||
| "tool_map": attr.label( | ||
| cfg = "exec", |
There was a problem hiding this comment.
Now that this is handled properly, we need to remove the exec transition in the cc_tool_map:
rules_cc/cc/toolchains/tool_map.bzl
Line 59 in e6d30a5
There was a problem hiding this comment.
Hmm might want to pick with a condition matching whether or not this is enabled (_bazel_version_ge("9.0.0-pre.20250911"))
To prevent duplication, it might be best to abstract that check with something like bazel_supports_starlarkified_cc_toolchians()
2 participants
However, it's not enough for
tool_mapto be oncc_toolchain_config- it must be on the toolchain itself to properly share the same exec platform as the action requesting the toolchain. Therefore we rearrange the implementation of thecc_toolchainmacro to pass it to the rule, and teach the rule to accept it from either the attribute or thecc_toolchain_config.Eventually,cc_toolchain_configcan be retired, though it will be a long journey.